Conversation
|
@microsoft-github-policy-service agree |
|
Thank you for this contribution, @cnukaus! 🎉 Adding Codex CLI support and fixing the Windows The |
Alan-Jowett
left a comment
Alan-Jowett
left a comment
There was a problem hiding this comment.
Thanks again for this well-structured PR! A few items to address before merge:
IMPLEMENTATION_PLAN.md — please remove (must fix)
This appears to be a development log from the implementation process. PromptKit doesn't maintain implementation plans at the repo root, and this would add noise to the project structure.
Spec files need codex added (should fix):
requirements.md has several references to the supported CLI list that need codex added:
- REQ-CLI-010 — detection order (
copilot → gh copilot → claude) - REQ-CLI-011 — valid
--clivalues (copilot, gh-copilot, claude) - REQ-CLI-017 — per-CLI spawn commands (missing codex entry)
- ASSUMPTION-003 — accepted
--clivalues
validation.md should document the new TC-CLI-072A test case alongside existing TC-CLI-072.
| assert.strictEqual( | ||
| parsedCmd, | ||
| `${cliName}.cmd`, | ||
| `${cliName} should spawn the Windows .cmd shim` |
There was a problem hiding this comment.
This assertion will fail on Windows — which is ironic given the PR's intent! 😄
The dry-run test uses an empty PATH directory (emptyBinDir), so when resolveSpawnCommand() runs, it can't find any .cmd file and correctly falls back to the bare command name. But this assertion expects ${cliName}.cmd on Windows.
To fix, create a .cmd stub in emptyBinDir so resolveSpawnCommand() can find it:
// Create a .cmd stub so resolveSpawnCommand finds it
if (process.platform === "win32" && cliName !== "gh-copilot") {
fs.writeFileSync(path.join(emptyBinDir, `${cliName}.cmd`), "");
}Alternatively, accept the bare command name when no .cmd shim is present on PATH (since that is the correct resolveSpawnCommand behavior).
There was a problem hiding this comment.
sure, this should have been fixed now.
| (Unix) — this is the most reliable cross-platform way to check if a | ||
| command exists on PATH without actually executing it. | ||
| - The detection order (copilot → gh-copilot → claude) prioritizes GitHub | ||
| - The detection order (copilot → gh-copilot → claude → codex) prioritizes GitHub |
There was a problem hiding this comment.
Pre-existing nit (optional fix): This line says detection uses execFileSync with where/which, but the implementation was changed to use direct PATH scanning in a previous PR. Since you're editing this section anyway, it might be worth correcting. Totally optional — not something you introduced.
There was a problem hiding this comment.
Hi, please check these should have been fixed
|
Updated routing .cmd launches through cmd.exe in cli/lib/launch.js:67 while keeping The No supported LLM CLI found on PATH lines are still expected from the negative-path tests. The important part is that the spawn EINVAL should now be gone. |
|
Hi @cnukaus / @what-cloud — thanks for the follow-up commits! A few items still need attention before we can merge:
Everything else looks good — the spec updates for codex, the test fixes, and the Windows |
3 participants
Enhanced Powershell support:
I added Windows command resolution helpers, added
codexdetection, expanded the “no CLI found” message to include Codex, and changed npm-installed CLIs to prefer<name>.cmdon Windows before spawning.That fixes the exact failure mode where PowerShell can run
codexbutchild_process.spawn(\"codex\")cannot.I also updated the CLI surface and docs, now advertises
codexin--cli